Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Try to prevent stungunning relayed peers #432

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

mawdegroot
Copy link

Filter the list of connected peers to exclude peers that do not have a public address per suggestion of @ci-work in #415

I don't think relayed nodes are the problem except the subset of relayed nodes that is running behind some CGNAT or similar system. The easiest way to avoid the issue is to stungun just those peers of which it is known they have a public ip address.

I have been running this change for almost 24h now and it hasn't yet picked up a relay address. I will deploy this to one more node and have it run over the weekend to limit the amount of luck involved.

Some potential problems I considered:

  1. What if there are only relayed peers connected to this node?
    It shouldn't matter since it will start a relay if it fails a few times in a row.

Addresses #415

@ci-work
Copy link
Contributor

ci-work commented Apr 22, 2022

can I suggest a modification, where has public and does not have relayed, as frequently due to this issue devices have both, e.g. 4.1k validators have a public IP, and 2.5k of them also have a relayed address right now.

@mawdegroot
Copy link
Author

I don't think it will matter since if it has a public ip address that will be the prioritized address that you connect to. That it also has a p2p address does not indicate that the peer itself has a broken internet configuration, just that it was hit by the same problem this commit tries to prevent.

I will run a second node with your suggestion and see if it makes any difference regardless 👍

@ci-work
Copy link
Contributor

ci-work commented Apr 22, 2022

thanks, and also for the additional insight!

@mawdegroot
Copy link
Author

The instance with this check has been running over the weekend without starting up a relay so this fix seems to prevent (at least a large part) of the issue. The second instance that also prevents selecting connected peers with a p2p address (as secondary address) achieved the same results so I propose to keep the change minimal and only check for a public IP.

Removing the draft label as the fix has had the intended result over a longer run.

@mawdegroot mawdegroot marked this pull request as ready for review April 25, 2022 06:57
@ci-work
Copy link
Contributor

ci-work commented Apr 25, 2022

confirming no issues in running, appears to resolve or at least help issue.

@ci-work
Copy link
Contributor

ci-work commented Jun 17, 2022

confirming still no issues, some weeks later

@jadeallenx jadeallenx merged commit 7a09441 into helium:master Jun 17, 2022
@Vagabond
Copy link
Contributor

This is actually wrong as libp2p_peer:connected_peers() doesn't return peerbook entries itself, but rather pubkey bins, which are not valid to call libp2p_peer:listen_addrs() on.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants